Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add section about user impact to PR template #8003

Merged
merged 4 commits into from
Nov 16, 2021

Conversation

sherifnada
Copy link
Contributor

@sherifnada sherifnada commented Nov 16, 2021

What

Add a section to the PR template which prompts the developer to think about the user impact of their change

the goal of this is to get the developer to think about the end impact this particular PR they are raising will have on the user especially if it contains a breaking change. If there is a breaking change, then we want to make sure it is:

  1. very thoroughly considered
  2. is communicated to users clearly perhaps via something like airbytehq/airbyte-internal-issues#331

this way, if there are any breaking issues or action required from the user, or just anything they really need to be aware of, this section will bring that to the forefront

@sherifnada sherifnada temporarily deployed to more-secrets November 16, 2021 00:46 Inactive
.github/pull_request_template.md Outdated Show resolved Hide resolved
@@ -9,12 +9,16 @@
1. `x.java`
2. `y.python`

## 🚨 User Impact 🚨
Are there any breaking changes?
What is the end result perceived by the user?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's probably more useful to put this in the connector README than in PR description? People usually won't check this.

Copy link
Contributor Author

@sherifnada sherifnada Nov 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the goal of this is to get the developer to think about the end impact this particular PR they are raising will have on the user especially if it contains a breaking change. If there is a breaking change, then we want to make sure it is:

  1. very thoroughly considered
  2. is communicated to users clearly perhaps via something like #8004

this way, if there are any breaking issues or action required from the user, or just anything they really need to be aware of, this section will bring that to the forefront.

as such, it feels like a README is the wrong place for this, as it is meant as a device to alert the developer that they need to think about the user

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Make sense.

I was actually thinking about the connector doc, but I wrote down "README" by mistake. It is probably also worth adding alert about breaking changes to the CHANGELOG section of the connector doc.

Co-authored-by: LiRen Tu <tuliren@gmail.com>
@sherifnada sherifnada temporarily deployed to more-secrets November 16, 2021 00:59 Inactive
## Pre-merge Checklist
Expand the relevant checklist and delete the others.

<details><summary> <strong> New Connector </strong></summary>
<p>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry that I did not mean to delete this whole line. Just wanted to remove the newly-introduced spaces.

@sherifnada sherifnada merged commit 90a5dfb into master Nov 16, 2021
@sherifnada sherifnada deleted the sherif/prtemplateuserimpact branch November 16, 2021 01:04
@sherifnada sherifnada temporarily deployed to more-secrets November 16, 2021 01:06 Inactive
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
Co-authored-by: LiRen Tu <tuliren@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants